Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement mandarins (WIP) #179

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

See issue #85

This is incomplete, I didn't work on this for months, but I want it here so that @ssiccha can see. Currently thing are way too slow and I don't yet understand why; I have the suspicion that PseudoRandom is too slow, but I do no yet understand why. This needs to be profiled and understood.

@ssiccha ssiccha self-assigned this Jul 24, 2020
@ssiccha
Copy link
Collaborator

ssiccha commented Oct 28, 2020

Rebased onto master and added a commit with a few comments.

@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #179 (1c1be29) into master (b1ee64a) will decrease coverage by 3.15%.
The diff coverage is 81.15%.

❗ Current head 1c1be29 differs from pull request most recent head 11b833e. Consider uploading reports for the commit 11b833e to get more accurate results

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
- Coverage   78.60%   75.45%   -3.16%     
==========================================
  Files          43       37       -6     
  Lines       18454    17240    -1214     
==========================================
- Hits        14506    13008    -1498     
- Misses       3948     4232     +284     
Impacted Files Coverage Δ
gap/base/recognition.gi 71.32% <80.88%> (+2.39%) ⬆️
gap/matrix.gi 83.17% <100.00%> (-11.26%) ⬇️
gap/matrix/classical.gi 52.06% <0.00%> (-23.19%) ⬇️
gap/projective/AnSnOnFDPM.gi 77.56% <0.00%> (-12.07%) ⬇️
gap/matrix/matimpr.gi 64.05% <0.00%> (-8.21%) ⬇️
gap/projective.gi 78.02% <0.00%> (-5.07%) ⬇️
gap/perm/giant.gi 89.88% <0.00%> (-3.17%) ⬇️
gap/base/methsel.gi 94.04% <0.00%> (-2.83%) ⬇️
gap/projective/classicalnatural.gi 93.13% <0.00%> (-0.98%) ⬇️
... and 28 more

@ssiccha ssiccha force-pushed the mh/mandarins branch 4 times, most recently from 9683114 to 39dd76f Compare December 10, 2020 10:18
@ssiccha
Copy link
Collaborator

ssiccha commented Dec 15, 2020

Rebased onto master and again added a few comments. I'm gonna look into some profiles now.

@ssiccha
Copy link
Collaborator

ssiccha commented Dec 16, 2020

I profiled a RecogniseGroup call using the group g from tst/working/quick/MatSn.tst. It looks like, without considering time spent in children, the functions SlotUsagePattern, ResultOfLineOfStraightLineProgram, and ResultOfStraightLineProgram contribute most of the overhead.

Maybe, if we rewrote those functions in C or in Julia, we would get rid of most of the overhead.

For the profiling runs and a comparison of the time spent per functions, see the funcoverview.html in each of the archives with-mandarins.tar.gz and without-mandarins.tar.gz.

@ssiccha
Copy link
Collaborator

ssiccha commented Dec 16, 2020

Also, BlocksModScalars does not seem to play nice with the mandarins but that's a whole other story.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 12, 2021

Continued work a bit and rebased onto master.

I simplified a few parts, but am not 100% sure that all simplifications are correct.

@ssiccha
Copy link
Collaborator

ssiccha commented Apr 1, 2021

rebased onto master

@ssiccha ssiccha marked this pull request as draft April 7, 2021 10:13
@ssiccha
Copy link
Collaborator

ssiccha commented Aug 23, 2021

Closed in favor of #280.

@ssiccha ssiccha closed this Aug 23, 2021
@ssiccha ssiccha reopened this Dec 28, 2021
@ssiccha
Copy link
Collaborator

ssiccha commented Dec 28, 2021

As @fingolfin mentioned in a call a few months ago, this will give us a reliable probability estimate as to whether the kernels were constructed correctly. Thus I'll continue work on this PR.

@ssiccha
Copy link
Collaborator

ssiccha commented Dec 29, 2021

Squashed and rebased onto master.

@ssiccha ssiccha force-pushed the mh/mandarins branch 4 times, most recently from 873155c to 949dce8 Compare January 12, 2022 15:27
@ssiccha
Copy link
Collaborator

ssiccha commented Jan 12, 2022

Rebased onto #308. That makes it easier for me to extend RecogniseGeneric.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 12, 2022

So, there are a few errors in the tests and tests now also take 21 minutes on my machine compared to roughly 13 minutes on the GitHub machines. It might have to do with me not storing the SLPs of the mandarins yet. I'll have a look at some profiles.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 19, 2022

I seem to not get errors anymore but instead there might be an infinite loop in tst/working/slow/C3C5.tst.

Also, I had a look at Mandarins in CompositionTree again. They compute SLPs for mandarins in the leaves. For mandarins in inner nodes, they from their SLPs from mandarin SLPs in the children. I think this is the magma line which does the job:

    node`MandarinSLPs :=
	[kernelNodeSLPs[i] * preImagesSLPs[i] : i in [1 .. #node`Mandarins]];

So if I understand correctly they create SLPs for the images, and then take each mandarin divided by a preimage of its image, and then find an SLP for that kernel element. In the end, glue everything together. Afterwards, they test whether the evaluating the SLP yields the original mandarin.

I bet this is a lot quicker than recomputing SLPs all the time, which is what I'm currently doing. First I need to get rid of this infinite loop though.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 19, 2022

The infinite loop was resolved by ignoring nodes recognised by BlocksModScalars. ri!.isone is broken for these nodes and that just doesn't work with mandarins.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 19, 2022

This leaves three open points:

  • how to design a dedicated test for the mandarin code?
  • handle TryToEnlargeKernelGeneratingSetAndUpdateSLPsDuringMandarinCrisis returning fail. There we have to discard and re-recognise the image recog node.
  • Handle the SLPs more intelligently, namely by storing and glueing them together.

@ssiccha
Copy link
Collaborator

ssiccha commented Jan 26, 2022

Of course there's also documentation! :D

And I vaguely remember something from an email by Eamonn O'Brien, I might be mixing up something though: if the SLPs in a computation become too long because we generated lots of random elements by product replacement, then it might make sense to reset the product replacement random generator of the group in question so that the SLPs become shorter again. 🤔

@ssiccha ssiccha force-pushed the mh/mandarins branch 2 times, most recently from 684cd2a to f7be240 Compare January 31, 2022 07:10
@ssiccha
Copy link
Collaborator

ssiccha commented Jan 31, 2022

I'm working on the SLP business. I expect tests to fail.

@ssiccha
Copy link
Collaborator

ssiccha commented Feb 24, 2022

I added a bunch more commits. Everything seems to be correct and the slow-down due to mandarins seems to be OK-ish for most tests. When we use mandarins immediate verification never finds incorrect kernels anymore, which makes sense since that's the whole point of the mandarins. So I'll see whether I can speed things up by disabling immediate verification when mandarins are enabled.

There are a few tests which become more than two times slower though. In some, mandarin crises get triggered (that is we found out that some kernel was too small) which leads to recomputing half the tree. I think I could improve upon that by being more conservative about which part of the tree gets thrown away during a crisis. If that doesn't resolve the reason for the crisis, we can still chop off more of the tree.

However, there also are a few tests which become super slow, even though no crisis was triggered. I'll have to look into that too.

Old commit msg of a commit which was squashed into this one and which
was nonsense:
RecogniseGeneric: recognise image only once

This is preparation for the implementation of Mandarins. The following
case is going to be handled by the Mandarins. Previously, if in an image
node writing of random elements as SLPs in the image node's nice
generators failed, that image node would be discarded and recomputed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants